Review: [recipes] OAuth MCP upgrade and remote auth hardening (#143)

48 files, ~8,400 additions. Single commit. Three-pass review: structured checklist, security adversarial, and completeness/logic analysis.

Scope: All files map to the 5 stated goals. No drift. Consider splitting into 2-3 PRs (auth code, auth portal, docs) for easier bisection.

CRITICAL — Fix before merge

1. Decision route has no auth check and no CSRF protection
dashboards/open-brain-auth-portal/app/api/oauth/decision/route.ts

The API route that approves/denies OAuth authorizations never calls getUser(). The consent page checks auth, but the endpoint doesn't. Combined with no CSRF protection, this is an account-takeover path:

• Attacker initiates their own OAuth flow → gets authorization_id
• Crafts a page with a hidden form that auto-POSTs decision=approve
• If the owner visits that page while logged into the auth portal, browser sends session cookies → authorization approved → attacker gets OAuth token

Fix: Add const { data: { user } } = await supabase.auth.getUser() at the top of the handler, return 401 if no user. Add Origin header validation for CSRF.

2. Open redirect on login page
app/login/page.tsx:10-11 + LoginForm.tsx:39

The redirect query param flows directly to router.push(redirectTo) with zero validation. /login?redirect=https://evil.com/phish redirects to attacker site after login. Can be chained with a fake consent page.

Fix: Validate redirectTo starts with / and doesn't contain :// or //.

3. shared-server.ts missing Accept header patch (real bug)
extensions/meal-planning/shared-server.ts

Every other MCP server includes the Claude Desktop Accept header workaround (if (!c.req.header('accept')?.includes('text/event-stream'))...). This file doesn't. Claude Desktop connections to the shared meal-planning endpoint will fail with a transport error.

Fix: Add the same Accept header patching block after the auth check, before transport.handleRequest(c).

4. Module-scoped McpServer with per-request reconnection (concurrency bug)
server/index.ts + integrations/kubernetes-deployment/index.ts

Both create McpServer at module scope but call server.connect(transport) on every request with a new transport. Under concurrent load, the second request's connect() can clobber the first request's transport. The 6 extensions avoid this by creating both McpServer and transport inside the request handler.

Fix: Move McpServer instantiation inside the request handler (matching the extension pattern that already works).

5. Timing-unsafe secret comparison (all server files)
server/index.ts:469, all 6 extensions (~line 108), integrations/kubernetes-deployment/index.ts:451,463

=== is not constant-time. For k8s this is the primary auth mechanism (not a legacy fallback).

Fix: Use crypto.subtle.timingSafeEqual().

6. Dedup SQL silently removed from getting-started guide (regression)
docs/01-getting-started.md

Removes content_fingerprint column, upsert_thought function, and dedup index from database setup. Added by merged PR #116, unrelated to OAuth. New users get incomplete database setup.

Fix: Restore the dedup SQL section.

INFORMATIONAL — Worth addressing

I1. ~120 lines auth middleware duplicated across 8 files — Any auth bug needs 8 fixes. Copies aren't identical (server returns ownerUserId, extensions don't, k8s has different logic entirely). A shared module via Deno import maps or build-time copy would help.

I2. K8s auth diverges from OAuth claim — Uses static Bearer token, no Supabase Auth, no .well-known endpoint. Reports auth: "bearer-token". Not wrong given k8s uses direct PostgreSQL, but needs explicit documentation.

I3. mode and ownerUserId return values are dead code — Never consumed by any caller. Remove or wire up.

I4. Error messages leak env var names — "SUPABASE_PUBLISHABLE_KEY not configured" tells attackers about infrastructure. Use generic errors in production.

I5. Client setup instructions gutted without replacements — Specific claude mcp add command, config.toml example, and bridge configs removed. Replaced with vague guidance.

I6. SQL interpolation for days in k8s — INTERVAL '${days} days' — Zod-validated but not parameterized. Defense in depth.

I7. "redirect_url" in authDetails duck-type check — Relies on undocumented Supabase return type discrimination. Fragile.

I8. shared-server.ts routes on /mcp while others use * — Undocumented divergence.

I9. Dashboard proxy forwards upstream error bodies — details: text in 502 responses could leak stack traces.

Overall

The core auth design is solid. Using authClient.auth.getUser(token) for server-side validation (checks revocation, not just JWT decode) is the right call. The legacy fallback gated by ALLOW_LEGACY_MCP_KEY is a clean migration path. The auth portal layout is clean and minimal.

The consent portal needs the auth check + CSRF fix (C1) and open redirect fix (C2) before merge. The shared-server.ts missing Accept patch (C3) will break Claude Desktop. The McpServer concurrency issue (C4) will manifest under any concurrent usage.

Nice contribution — the OAuth migration recipe is well-written and the credential tracker pattern is a good UX touch.